[6.x] Guard ImageGenerator against a null asset - #15447
wakqasahmed wants to merge 6 commits into
Conversation
…c#15359) When an asset URL resolves to null (e.g. a repository that can't find the asset by that URL, such as statamic/eloquent-driver#609), generateByAsset() dereferenced it directly, throwing an uncaught Error rather than the Exception the glide tag already knows how to catch and skip. Return '' early instead, matching the existing skip convention used elsewhere in this method.
| */ | ||
| public function generateByAsset($asset, array $params) | ||
| { | ||
| if (! $asset) { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…null-asset # Conflicts: # src/Imaging/ImageGenerator.php # tests/Imaging/ImageGeneratorTest.php
jasonvarga
changed the title
duncanmcclean
changed the title
| */ | ||
| public function generateByAsset($asset, array $params) | ||
| { | ||
| if (! $asset) { |
There was a problem hiding this comment.
The new Log::error('Cannot generate an image for a missing asset.') carries no identifying context (no item, path, or asset ID). The original issue's complaint was that a bad asset silently 500'd for a day before anyone noticed; this line does add a log, but by the time generateByAsset() sees $asset === null, the identifying info from the caller ($item in Glide::generateGlideUrl/generateImage) is already gone. Every other failure path in Glide::generate() logs $e->getMessage(), which carries exception context — this one is a flat string that will look identical for every occurrence, so it's still not possible to tell which asset/URL is failing without reproducing it.\n\nWorth either passing/logging the original $item reference here, or (per the earlier thread on this line) throwing from Glide::generateImage() so the existing catch (\\Exception $e) logs a message that includes the offending item.
…lved
Log::error() inside ImageGenerator::generateByAsset()'s null guard has
no way to know which item the caller was resolving, so every
occurrence logged an identical, context-free message. Throw instead
from Glide::generateImage() (where $item is still in scope) so the
tag's existing catch (\Exception $e) { Log::error($e->getMessage()); }
in generate() logs a message that includes the offending item.
Per @jasonvarga's follow-up review on statamic#15447.
| */ | ||
| public function generateByAsset($asset, array $params) | ||
| { | ||
| if (! $asset) { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
…null-asset # Conflicts: # src/Tags/Glide.php
|
Closing this since #15360 fixed #15359 with the |
3 participants
Closes #15359
ImageGenerator::generateByAsset()calls$asset->isVideo()immediately, so when an asset URL resolves tonull(e.g. a repository that can't find the asset by that URL — statamic/eloquent-driver#609 is one real cause) it throwsError: Call to a member function isVideo() on nullinstead of returning gracefully. Because that's an\Error, not an\Exception, it isn't caught by the glide tag's existingcatch (\Exception)handling, so one unresolvable asset 500s the whole page instead of just being skipped.Added an early return of
''when$assetis falsy, matching the method's existing pattern of returning''to skip (see theisVideo()branch just below it). Added a regression test forgenerateByAsset(null, [...])and confirmed it throws the exact error from the issue before the fix and passes after. Ran the fulltests/Imaging+tests/Tags/GlideTest.phpsuites (147 tests, 297 assertions) and Pint, both clean.